Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix integer overflow in leap year calculation #1955

Merged
merged 2 commits into from
Oct 16, 2021

Conversation

kevinbackhouse
Copy link
Collaborator

Fixes: #1954

Credit to OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39555

This code looks completely bogus to me. I can't find any reference online that says that negative years should be treated different in a leap year calculation. So the simplest solution to the integer overflow bug is to comment this code out.

@codecov
Copy link

codecov bot commented Oct 10, 2021

Codecov Report

Merging #1955 (a3fe7c8) into main (937264b) will increase coverage by 0.00%.
The diff coverage is 78.57%.

❗ Current head a3fe7c8 differs from pull request most recent head 4bc4757. Consider uploading reports for the commit 4bc4757 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1955   +/-   ##
=======================================
  Coverage   61.13%   61.13%           
=======================================
  Files          96       96           
  Lines       19068    19054   -14     
  Branches     9745     9732   -13     
=======================================
- Hits        11657    11649    -8     
+ Misses       5093     5092    -1     
+ Partials     2318     2313    -5     
Impacted Files Coverage Δ
include/exiv2/types.hpp 70.58% <ø> (+1.14%) ⬆️
src/actions.hpp 100.00% <ø> (ø)
src/makernote_int.cpp 67.62% <ø> (ø)
src/bmffimage.cpp 71.42% <50.00%> (ø)
src/image.cpp 65.09% <50.00%> (ø)
src/tiffvisitor_int.cpp 76.23% <50.00%> (ø)
src/actions.cpp 61.13% <57.14%> (ø)
src/crwimage_int.hpp 84.61% <71.42%> (+0.24%) ⬆️
src/types.cpp 87.98% <84.61%> (-0.46%) ⬇️
src/crwimage_int.cpp 74.07% <92.85%> (+0.03%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 937264b...4bc4757. Read the comment docs.

@kmilos
Copy link
Collaborator

kmilos commented Oct 11, 2021

Indeed, see https://en.wikipedia.org/wiki/Leap_year#Gregorian_calendar (and the "Algorithm" subsection).

Maybe the comment should also say something like "Assuming Gregorian calendar since 1582; years before are assumed to be using the proleptic Gregorian calendar and years <1 are assumed to be using astronomical year numbering"

@kmilos
Copy link
Collaborator

kmilos commented Oct 11, 2021

See also https://en.wikipedia.org/wiki/ISO_8601#Years basically saying the convention for any year before 1582 has to be clearly agreed/identified between communicating parties.

@clanmills
Copy link
Collaborator

If we get a contributor from Vatican City, we could ask her to deal with this.

xmpsdk/src/XMPUtils.cpp Outdated Show resolved Hide resolved
@kevinbackhouse kevinbackhouse merged commit d23ef9d into Exiv2:main Oct 16, 2021
@kevinbackhouse kevinbackhouse deleted the FixIssue1954 branch October 16, 2021 22:26
@kevinbackhouse kevinbackhouse added this to the v1.00 milestone Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UBSAN error due to integer overflow in leap year calculation
3 participants